-
Notifications
You must be signed in to change notification settings - Fork 369
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add validator:status command to check if a validator signer is online and producing blocks #1906
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1906 +/- ##
========================================
Coverage 74.42% 74.42%
========================================
Files 281 281
Lines 7824 7824
Branches 687 975 +288
========================================
Hits 5823 5823
Misses 1884 1884
Partials 117 117
Continue to review full report at Codecov.
|
I've added a note to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check lodash, i don't think it's included in package.json...
The rest are minor details!
// Use redundant checks to help the user diagnose issues. | ||
await newCheckBuilder(this, signer) | ||
.canSignValidatorTxs() | ||
.signerMeetsValidatorBalanceRequirements() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect us to fail this check whenever the validator has an authorized validator signer...
Where else is this used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's used here. https://github.com/celo-org/celo-monorepo/blob/master/packages/cli/src/commands/validator/register.ts#L32
I believe it should work. It calls signerToAccount
on the provided signer then checks the balance requirement on the account that was retrieved. It's not strictly necessary though.
const signers = await election.getCurrentValidatorSigners() | ||
const signerIndex = signers.map((a) => a.toLowerCase()).indexOf(signer.toLowerCase()) | ||
if (signerIndex < 0) { | ||
// Determine whether the signer will be elected at the next epoch to provide a helpful error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! Can we put this functionality in the election
module instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could add a command like election:validator-is-elected
, but I personally don't see a ton of value in that because election:current
and election:run
exist and combined with grep you can tell whether a specific validator is in the set.
…into victor/vx-online
…into victor/vx-online
static flags = { | ||
...BaseCommand.flags, | ||
signer: Flags.address({ | ||
description: 'address of the validator to check if elected and validating', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these descriptions swapped?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Fixed.
|
||
static flags = { | ||
...BaseCommand.flags, | ||
signer: Flags.address({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make these flags exclusive? Example in validatorgroup:member
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
const account = res.flags.validator | ||
checker.isValidator(account).meetsValidatorBalanceRequirements(account) | ||
} else if (res.flags.signer) { | ||
checker.signerMeetsValidatorBalanceRequirements().signerAccountIsValidator() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than add additional checks, why don't we just pull the account
from the signer and run the same checks on the account?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point in the code it is unclear whether the accounts given are valid and I wanted to avoid an opaque call reversion. I realized that as coded it still has that issue, so I've added an isAccount
check.
const frontrunners = await election.electValidatorSigners() | ||
if (frontrunners.some((a) => eqAddress(a, signer))) { | ||
this.error( | ||
`Signer ${signer} is not elected for this epoch, but is currently winning in the upcoming election. Wait for the next epoch.` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but would be elected to the validator set for the next epoch if an election were to be held now. Please wait until the next epoch.
) | ||
} else { | ||
this.error( | ||
`Signer ${signer} is not elected for this epoch, and is not currently winning the upcoming election.` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and would not be elected to the validator set for the next epoch if an election were to be held now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
…into victor/vx-online
…g/celo-monorepo into aaronmgdr/protect-proxied-routes * 'aaronmgdr/protect-proxied-routes' of github.com:celo-org/celo-monorepo: Add validator:status command to check if a validator signer is online and producing blocks (#1906)
* master: (208 commits) Fix potential hard timeouts (#2072) Add checkzero,blockscout,mulit-address support to faucet. Fix broken envType checks (#2068) README added mission (#1972) Add dev-utils README (#2081) Add validator:status command to check if a validator signer is online and producing blocks (#1906) Experience Brand Kit 1.0 (#1948) Adjust reference to the rewards app (#2065) [Wallet] Compatibility with exchange rate in string format (#2060) Fix Typo in CI config (#2056) Fix additional attestations instructions (#2057) Allow a specified address to disable/enable rewards distribution (#1828) Aaronmgdr/leaderboard patch (#2055) Move attestation service instructions to main page (#2051) Point To Updated Join Celo Video (#2052) Fix minor issue withe the ordering of instructions changes to docs related to discovery (#2025) [Docs] Fix typos in Running a Validator docs (#2045) Add node flag to celocli to set the target node for a single command (#2020) Fix broken links and spruce up CLI docs for accounts command (#2027) Prevent clipping of arrow component (#2036) ... # Conflicts: # packages/web/src/layout/BookLayout.tsx # packages/web/src/lib.dom.d.ts
Description
Create a
validator:status
command to check the registration, election, and signing status of a validator.Tested
Manually tested against local testnet
Backwards compatibility
Will not break anything. New command will work with networks that have parent seals in the block header.
Related Issues
Fixes: #1782